implement ResourceList component with agencies pages (DEV-77)#14
implement ResourceList component with agencies pages (DEV-77)#14
Conversation
ea7e9ce to
c27853f
Compare
cbf380b to
a117e60
Compare
nuthanan06
left a comment
There was a problem hiding this comment.
just a couple changes on moving some things into other files + delete hasn't been implemented yet, otherwise lgtm! 🔥
a117e60 to
6e79ad6
Compare
6e79ad6 to
784e365
Compare
784e365 to
d49f48b
Compare
435bec2 to
1a294e2
Compare
|
|
||
| case "text": | ||
| default: | ||
| return value != null ? String(value) : "—"; |
There was a problem hiding this comment.
nit: this is the only branch that returns a plain string instead of JSX, potentially wrap (<>{value != null ? String(value) : "—"}</>) so it's consistent with the other cases
| import Link from "next/link"; | ||
| import { useParams, useRouter } from "next/navigation"; | ||
| import { useAgency } from "@/hooks/useApi"; | ||
| import type { Agency } from "@/types"; |
There was a problem hiding this comment.
looks like Agency isn't used and should be safe to remove
| return ( | ||
| <div className="w-full" data-testid={testId}> | ||
| {/* Error State */} | ||
| {error && ( |
There was a problem hiding this comment.
if a failed request is retried, error and loading might both be true at the same time - can we add && !loading to avoid rendering both states at the same time
| export default function AgencyEditPage() { | ||
| const params = useParams(); | ||
| const router = useRouter(); | ||
| const agencyId = params.id as string; |
There was a problem hiding this comment.
consider checking that params.id is a non-empty string before using it, and explicitly handle missing/invalid cases (e.g. render an error) instead of just casting
| export default function AgencyDetailPage() { | ||
| const params = useParams(); | ||
| const router = useRouter(); | ||
| const agencyId = params.id as string; |
There was a problem hiding this comment.
same params.id comment as left in frontend/app/agencies/[id]/edit/page.tsx
| <div className="min-h-screen flex flex-col"> | ||
| <header className="w-full bg-gradient-to-r from-blue-600 to-blue-800 text-white shadow-lg"> | ||
| <div className="max-w-6xl mx-auto px-6 py-6 flex justify-between items-center"> | ||
| <h1 className="text-2xl font-bold">Loading…</h1> | ||
| <button | ||
| onClick={() => router.back()} | ||
| className="px-4 py-2 bg-white/20 rounded hover:bg-white/30 transition" | ||
| > |
There was a problem hiding this comment.
we seem to be duplicating the full page layout multiple times in this file - let's extract a shared layout component
|
|
||
| if (isLoading) { | ||
| return ( | ||
| <div className="min-h-screen flex flex-col"> |
There was a problem hiding this comment.
same layout duplication as in frontend/app/agencies/[id]/edit/page.tsx
| beforeEach(() => { | ||
| (useAgencies as jest.Mock).mockReset(); | ||
| (useDeleteAgency as jest.Mock).mockReturnValue({ mutate: jest.fn() }); | ||
| jest.spyOn(global, "confirm").mockReturnValue(true); |
There was a problem hiding this comment.
it doesn't look like we ever restore the confirm spy - let's add something like afterEach(() => jest.restoreAllMocks()) to prevent the mock from leaking into other tests
| }, | ||
| staleTime: 1000 * 60 * 5, |
There was a problem hiding this comment.
| }, | |
| staleTime: 1000 * 60 * 5, | |
| }, | |
| enabled: Boolean(agencyId), | |
| staleTime: 1000 * 60 * 5, |
There was a problem hiding this comment.
this should tell the query to wait until there's a real value before making any network calls
Jira ticket link
DEV-77
Implementation description
Steps to test
What should reviewers focus on?
Checklist
Format for branch, commit, and PR title: docs/GIT.md.